apps/server/src/detectedServers/Layers/SocketProbe.Linux.ts (1)

88-111: 💤 Low value

Consider avoiding non-null assertion for clarity.

Line 110 uses a non-null assertion (!) after checking inodeToPid.has(r.inode) on line 109. While safe, using a more explicit pattern improves readability and type safety.

♻️ Optional refinement

     const rows = [...parseProcTcpRows(tcpText), ...parseProcTcp6Rows(tcp6Text)];
-    return rows
-      .filter((r) => r.state === "LISTEN" && inodeToPid.has(r.inode))
-      .map((r) => ({ pid: inodeToPid.get(r.inode)!, port: r.port, host: r.host }));
+    const results: ProbeResult[] = [];
+    for (const r of rows) {
+      if (r.state === "LISTEN") {
+        const pid = inodeToPid.get(r.inode);
+        if (pid !== undefined) results.push({ pid, port: r.port, host: r.host });
+      }
+    }
+    return results;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/server/src/detectedServers/Layers/SocketProbe.Linux.ts` around lines 88
- 111, The code in probeImpl uses a non-null assertion inodeToPid.get(r.inode)!
after checking inodeToPid.has(r.inode); replace this with an explicit safe
lookup to avoid the bang: when building the result from rows, obtain const pid =
inodeToPid.get(r.inode) and only return entries when pid is defined (or use a
flatMap/filter that discards undefined pids) so the final map uses the real pid
variable instead of r.inode!; update references in the rows -> filter/map
pipeline (rows, inodeToPid, r.inode) accordingly to preserve types and
readability.

apps/server/src/detectedServers/Layers/SocketProbe.Darwin.ts (1)

5-29: 💤 Low value

Consider adding a comment for the parts.length - 1 slice.

Line 14 slices to parts.length - 1 to exclude the trailing "(LISTEN)" token from the name field. While correct, a brief inline comment would improve readability for future maintainers.

📝 Suggested comment

-    const nameField = parts.slice(8, parts.length - 1).join(" ");
+    // Exclude trailing "(LISTEN)" from name field
+    const nameField = parts.slice(8, parts.length - 1).join(" ");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/server/src/detectedServers/Layers/SocketProbe.Darwin.ts` around lines 5
- 29, Add a brief inline comment explaining why parts.slice(8, parts.length - 1)
excludes the trailing "(LISTEN)" token when building nameField in
parseLsofOutput; place the comment near the slice expression (in the
parseLsofOutput function) noting that the last token is "(LISTEN)" and should
not be included in the name/host:port parsing so future readers understand the
-1 adjustment.

apps/server/src/detectedServers/StdoutSniffer.test.ts (2)

94-100: ⚡ Quick win

Add length assertion before accessing out[0].

The ANSI stripping test accesses out[0]! without first asserting the array has at least one element. Add expect(out).toHaveLength(1); before line 99 for consistency with the chunked URL test above.

📋 Proposed fix

   it("strips ANSI before matching", () => {
     const sniffer = new StdoutSniffer();
     const out: { url: string }[] = [];
     sniffer.onCandidate((c) => out.push(c));
     sniffer.feed("\x1b[36m  ➜  Local:\x1b[0m \x1b[1mhttp://localhost:5173/\x1b[0m\n");
+    expect(out).toHaveLength(1);
     expect(out[0]!.url).toBe("http://localhost:5173/");
   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/server/src/detectedServers/StdoutSniffer.test.ts` around lines 94 - 100,
In the "strips ANSI before matching" test for StdoutSniffer, add an assertion
that the out array has one element before accessing out[0]; specifically, insert
expect(out).toHaveLength(1) immediately after feeding the sniffer and before the
existing expect(out[0]!.url).toBe(...) so the test checks presence of the
candidate before indexing.

---

21-82: ⚡ Quick win

Add length assertions before accessing array elements.

Most tests directly access out[0]! without first asserting the array length. If the sniffer fails to detect a URL, the test will fail with an unclear "Cannot read properties of undefined" error instead of a descriptive assertion failure. The Vite test (line 15) demonstrates the correct pattern with expect(out).toHaveLength(1) before accessing elements.

📋 Proposed fix to add length checks

   it("extracts Next URL", () => {
     const sniffer = new StdoutSniffer();
     const out: { url: string; port: number; framework: string }[] = [];
     sniffer.onCandidate((c) => out.push(c));
     sniffer.feed(readFileSync(fixturePath("next"), "utf8"));
+    expect(out).toHaveLength(1);
     expect(out[0]!.url).toBe("http://localhost:3000");
     expect(out[0]!.framework).toBe("next");
   });

Apply the same pattern to Nuxt (line 30+), Astro (line 39+), Remix (line 48+), Wrangler (line 57+), Webpack (line 66+), and Express (line 75+) tests.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/server/src/detectedServers/StdoutSniffer.test.ts` around lines 21 - 82,
Tests for StdoutSniffer access out[0] without asserting the array length, which
can produce unclear errors; before any expect that reads out[0] (in the Nuxt,
Astro, Remix, Wrangler, Webpack and Express tests) add an assertion like
expect(out).toHaveLength(1) so the test fails with a clear message if no
candidate was emitted, then proceed to assert out[0]!.url / .port / .framework
as before.

apps/server/src/detectedServers/ArgvHinter.test.ts (1)

1-98: ⚡ Quick win

Add test coverage for shortcut invocation (yarn/pnpm/bun).

The code at lines 64-73 in ArgvHinter.ts supports shortcut invocations like yarn dev, pnpm dev, and bun dev (without the explicit run keyword). This code path is currently untested. Add test cases to verify this behavior.

📋 Suggested test cases

  it("treats npm run build as build, not server", () => {
    const got = hintFromArgv(["npm", "run", "build"], { scripts: { build: "vite build" } });
    expect(got).toEqual({ framework: "vite", isLikelyServer: false });
  });

+  it("expands yarn dev shortcut (without explicit run)", () => {
+    const got = hintFromArgv(["yarn", "dev"], { scripts: { dev: "vite" } });
+    expect(got).toEqual({ framework: "vite", isLikelyServer: true });
+  });
+
+  it("expands pnpm start shortcut", () => {
+    const got = hintFromArgv(["pnpm", "start"], { scripts: { start: "next dev" } });
+    expect(got).toEqual({ framework: "next", isLikelyServer: true });
+  });
+
+  it("expands bun serve shortcut", () => {
+    const got = hintFromArgv(["bun", "serve"], { scripts: { serve: "astro dev" } });
+    expect(got).toEqual({ framework: "astro", isLikelyServer: true });
+  });

  // Regression tests: prefix-match false positives

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/server/src/detectedServers/ArgvHinter.test.ts` around lines 1 - 98, Add
tests covering shortcut invocations (yarn/pnpm/bun) to exercise hintFromArgv's
branch for commands like "yarn dev" (no "run"); specifically add cases that call
hintFromArgv(["yarn","dev"], {scripts:{dev:"vite"}}),
hintFromArgv(["pnpm","dev"], {scripts:{dev:"vite"}}) and
hintFromArgv(["bun","dev"], {scripts:{dev:"vite"}}) and assert they return {
framework: "vite", isLikelyServer: true }, plus at least one build shortcut test
(e.g. ["yarn","build"] with scripts.build "vite build") asserting { framework:
"vite", isLikelyServer: false }; reference hintFromArgv in ArgvHinter.test.ts
and add the new it(...) blocks alongside the existing tests.

apps/web/src/components/BranchToolbar.tsx (1)

134-134: ⚡ Quick win

Avoid creating a new function on every render.

The pattern onClick={onOpenServersTab ?? (() => {})} creates a new no-op function each time BranchToolbar re-renders when onOpenServersTab is undefined. If DetectedServersBadge is memoized, this will cause unnecessary child re-renders.

Suggested refactor

Option 1: Use a stable NOOP constant

+const NOOP = () => {};
+
 export const BranchToolbar = memo(function BranchToolbar({
   ...
 }: BranchToolbarProps) {
   ...
-        <DetectedServersBadge servers={detectedServers} onClick={onOpenServersTab ?? (() => {})} />
+        <DetectedServersBadge servers={detectedServers} onClick={onOpenServersTab ?? NOOP} />

Option 2: Make onClick optional in DetectedServersBadge and conditionally render the badge

-        <DetectedServersBadge servers={detectedServers} onClick={onOpenServersTab ?? (() => {})} />
+        {onOpenServersTab && <DetectedServersBadge servers={detectedServers} onClick={onOpenServersTab} />}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/components/BranchToolbar.tsx` at line 134, BranchToolbar
currently passes onClick={onOpenServersTab ?? (() => {})}, which allocates a new
noop on every render and can break DetectedServersBadge memoization; fix by
using a stable NOOP constant (e.g., define a module-level NOOP and pass that
instead) or make DetectedServersBadge's onClick prop optional and only pass
onOpenServersTab when it's defined (or conditionally render the badge),
referencing DetectedServersBadge and the onOpenServersTab prop in BranchToolbar
to locate the change.

apps/web/src/terminalStateStore.ts (1)

758-855: ⚡ Quick win

Keep terminalDrawerKindByThreadKey in sync with terminal state cleanup

Line 758 onward clears/removes thread-scoped terminal state, launch context, and event buffers, but leaves terminalDrawerKindByThreadKey untouched. This creates stale per-thread UI state and unbounded growth over time.

Suggested fix

         clearTerminalState: (threadRef) =>
           set((state) => {
             const threadKey = terminalThreadKey(threadRef);
@@
             const { [threadKey]: _removed, ...remainingLaunchContexts } =
               state.terminalLaunchContextByThreadKey;
+            const { [threadKey]: _removedDrawerKind, ...remainingDrawerKinds } =
+              state.terminalDrawerKindByThreadKey;
@@
             return {
               terminalStateByThreadKey: nextTerminalStateByThreadKey,
               terminalLaunchContextByThreadKey: remainingLaunchContexts,
+              terminalDrawerKindByThreadKey: remainingDrawerKinds,
               terminalEventEntriesByKey: nextTerminalEventEntriesByKey,
             };
           }),
         removeTerminalState: (threadRef) =>
           set((state) => {
@@
             const nextLaunchContexts = { ...state.terminalLaunchContextByThreadKey };
             delete nextLaunchContexts[threadKey];
+            const nextDrawerKinds = { ...state.terminalDrawerKindByThreadKey };
+            delete nextDrawerKinds[threadKey];
             return {
               terminalStateByThreadKey: nextTerminalStateByThreadKey,
               terminalLaunchContextByThreadKey: nextLaunchContexts,
+              terminalDrawerKindByThreadKey: nextDrawerKinds,
               terminalEventEntriesByKey: nextTerminalEventEntriesByKey,
             };
           }),
         removeOrphanedTerminalStates: (activeThreadKeys) =>
           set((state) => {
@@
             const orphanedLaunchContextIds = Object.keys(
               state.terminalLaunchContextByThreadKey,
             ).filter((key) => !activeThreadKeys.has(key));
+            const orphanedDrawerKindIds = Object.keys(
+              state.terminalDrawerKindByThreadKey,
+            ).filter((key) => !activeThreadKeys.has(key));
@@
             if (
               orphanedIds.length === 0 &&
               orphanedLaunchContextIds.length === 0 &&
+              orphanedDrawerKindIds.length === 0 &&
               !removedEventEntries
             ) {
               return state;
             }
@@
             const nextLaunchContexts = { ...state.terminalLaunchContextByThreadKey };
             for (const id of orphanedLaunchContextIds) {
               delete nextLaunchContexts[id];
             }
+            const nextDrawerKinds = { ...state.terminalDrawerKindByThreadKey };
+            for (const id of orphanedDrawerKindIds) {
+              delete nextDrawerKinds[id];
+            }
             return {
               terminalStateByThreadKey: next,
               terminalLaunchContextByThreadKey: nextLaunchContexts,
+              terminalDrawerKindByThreadKey: nextDrawerKinds,
               terminalEventEntriesByKey: nextTerminalEventEntriesByKey,
             };
           }),

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/terminalStateStore.ts` around lines 758 - 855, The cleanup
functions clearTerminalState, removeTerminalState, and
removeOrphanedTerminalStates must also remove per-thread entries from
terminalDrawerKindByThreadKey to avoid stale UI state: for clearTerminalState
and removeTerminalState compute whether
state.terminalDrawerKindByThreadKey[threadKey] existed (hadDrawer) and create a
shallow copy nextDrawerKinds from state.terminalDrawerKindByThreadKey, delete
nextDrawerKinds[threadKey], include nextDrawerKinds in the returned object, and
include hadDrawer in the early-return condition; for
removeOrphanedTerminalStates compute orphaned drawer keys from
Object.keys(state.terminalDrawerKindByThreadKey) that are not in
activeThreadKeys, delete them from a copy nextDrawerKinds, include
nextDrawerKinds in the returned state, and include their presence in the
early-return check.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

apps/server/src/detectedServers/Layers/DetectedServersIngress.ts (1)

16-23: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bare --inspect flags are not currently captured—apply the suggested fix.

The argvHasInspect function only matches --inspect=PORT forms (with explicit =PORT). Bare flags like --inspect, --inspect-brk, and --inspect-wait without an explicit port are silently ignored, even though they should default to port 9229 per Node.js behavior. This breaks debugger port detection for common use cases.

The regex at line 19 should be updated to handle both forms:

Suggested fix

 const argvHasInspect = (argv: ReadonlyArray<string>): number[] => {
   const out: number[] = [];
   for (const t of argv) {
+    if (/^--inspect(?:-brk|-wait)?$/.test(t)) {
+      out.push(9229);
+      continue;
+    }
     const m = t.match(/--inspect(?:-brk|-wait)?=(?:[^:]*:)?(\d+)/);
     if (m) out.push(Number.parseInt(m[1]!, 10));
   }
   return out;
 };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/server/src/detectedServers/Layers/DetectedServersIngress.ts` around
lines 16 - 23, The argvHasInspect function currently only matches --inspect
forms with an explicit "=PORT"; update its pattern and logic in argvHasInspect
to also detect bare flags (--inspect, --inspect-brk, --inspect-wait) and treat
them as port 9229 when no port is provided: modify the regex used in
argvHasInspect to allow an optional "=(...)" capture and then if the capture is
missing/empty push 9229, otherwise parse the captured port number as before
(refer to argvHasInspect and the variable m in the loop).

apps/server/src/detectedServers/DetectedServersIngress.test.ts (1)

76-77: 💤 Low value

Add length assertion before accessing array element.

The test accesses updated[0]! without first asserting updated.toHaveLength(1). If the test fails with an empty array, the error message will be less clear.

♻️ Clearer test structure

         const updated = yield* registry.getCurrent("t-pty");
+        expect(updated).toHaveLength(1);
         expect(updated[0]!.status === "candidate" || updated[0]!.status === "live").toBe(true);
         expect(updated[0]!.port).toBe(5173);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/server/src/detectedServers/DetectedServersIngress.test.ts` around lines
76 - 77, The test reads updated[0]! without asserting the array length, which
can cause unclear failures; add an explicit assertion like
expect(updated).toHaveLength(1) (or appropriate expected length) before
accessing updated[0]!, then keep the existing assertions on updated[0]!.status
and updated[0]!.port to ensure clear, early failure if the array is empty.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

apps/web/src/components/detectedServers/DetectedServersPanel.test.tsx (2)

69-74: ⚡ Quick win

Remove placeholder test or implement proper handler verification.

This test verifies that renderToStaticMarkup doesn't call handlers—a documented behavior of the API. It adds no coverage value and the title acknowledges it's a placeholder.

Either remove it or use @testing-library/react with render() and fireEvent to verify the actual onStop handler wiring.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/components/detectedServers/DetectedServersPanel.test.tsx` around
lines 69 - 74, The placeholder test "vi noop sanity (placeholder for future
onStop handler verification)" provides no value—either delete this test or
replace it with a real interaction test: use `@testing-library/react`'s render()
instead of renderToStaticMarkup via renderView, locate the stop control in the
rendered output (e.g., by role/text), call fireEvent.click or userEvent.click,
and assert that the onStop mock (created by vi.fn()) was called; reference the
existing helpers make, renderView and the onStop mock when making the change.

---

60-67: ⚡ Quick win

Consider more robust assertions for styling verification.

The test counts bg-accent occurrences in the static HTML string, which tightly couples the test to Tailwind implementation details. This will break if class names change or if the CSS approach is refactored (e.g., CSS-in-JS, CSS modules).

Consider using @testing-library/react with render() instead of renderToStaticMarkup to verify computed styles or DOM structure more robustly.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/components/detectedServers/DetectedServersPanel.test.tsx` around
lines 60 - 67, The test should stop counting occurrences in the HTML string and
instead render the component with `@testing-library/react` and assert DOM/classes
directly: update renderView to use render() (from `@testing-library/react`) rather
than renderToStaticMarkup, render the rows created by make({ id }) and then
query the active row (e.g., getByText or a data-testid on the row) and assert
the element hasClass 'bg-accent' (use expect(element).toHaveClass('bg-accent'))
and that a non-active row does not have the bare class; this makes the test
resilient to changes in static markup or CSS build details.

apps/web/src/components/detectedServers/DetectedServerRow.test.tsx (3)

60-61: 💤 Low value

Regex assumes attribute ordering in static markup.

The regex /aria-label="Stop"[^>]* disabled=""/ requires aria-label to appear before disabled in the HTML. While renderToStaticMarkup currently has consistent attribute ordering, this assumption is fragile.

Consider a more robust approach like:

• Checking for both attributes independently
• Using a lightweight HTML parser
• Switching to @testing-library/react which provides toBeDisabled() matcher

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/components/detectedServers/DetectedServerRow.test.tsx` around
lines 60 - 61, The helper stopBtnDisabled currently uses a fragile regex that
assumes aria-label appears before disabled; update the test to locate the
element and check the disabled state instead of relying on attribute order —
replace the stopBtnDisabled(regex) approach by rendering the component
(renderToStaticMarkup or better, use `@testing-library/react`'s render) and query
the element with aria-label "Stop" (e.g., getByLabelText/queryByLabelText) then
assert its disabled property or use the toBeDisabled() matcher; alternatively,
if you must work with static markup, parse markup to find the element and check
for the presence of the disabled attribute independently of attribute order
(modify the stopBtnDisabled function to test for both aria-label="Stop" and
disabled separately).

---

75-82: ⚡ Quick win

Consider more robust assertions for styling verification.

Counting bg-accent occurrences tightly couples the test to Tailwind class implementation. This mirrors the same brittleness in DetectedServersPanel.test.tsx. Consider using @testing-library/react with render() to verify computed styles or DOM structure instead.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/components/detectedServers/DetectedServerRow.test.tsx` around
lines 75 - 82, The test in DetectedServerRow.test.tsx currently uses a brittle
regex count on renderRow(...) output; update the test to render the component
with `@testing-library/react` (or make renderRow return the render container) and
assert against the DOM/classList instead of string matches: locate the renderRow
helper and make it return the render() result or container, then in the test use
screen/getByRole or container.querySelector to find the row/button element
produced by make(...) and assert element.classList.contains('bg-accent') for the
active case and not for the inactive case (or assert exact classList length if
you need to verify order), replacing the regex expectations with these DOM-based
assertions.

---

32-44: ⚡ Quick win

Consider testing visual behavior rather than Tailwind class names.

The test directly asserts Tailwind class strings (e.g., bg-blue-500/20), coupling tests to implementation details. If styling moves to CSS-in-JS, CSS modules, or class names change, these tests will break despite correct visual output.

For a more robust approach, consider using @testing-library/react to verify computed styles or semantic attributes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/web/src/components/detectedServers/DetectedServerRow.test.tsx` around
lines 32 - 44, The test DetectedServerRow.test.tsx is brittle because it asserts
Tailwind class strings; update the it.each block that calls renderRow(make({
status })) to assert visual/semantic output instead of class names: locate
DetectedServerRow (and helper renderRow/make) and change the assertions to query
the status pill element (by role/text/data-testid) and verify its computed style
(e.g., backgroundColor via window.getComputedStyle) or a semantic attribute
(aria-label or title) that reflects the status; alternatively add a stable
data-testid on the pill in DetectedServerRow and assert computed styles or
status text so tests don't depend on specific Tailwind class names.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes